Repository navigation
Conversation
This comment has been minimized.
This comment has been minimized.
|
@Arawoof06 please rebase. |
d4485f1 to
1e2344a
Compare
|
Rebased on main, should be good now. |
1e2344a to
611438c
Compare
|
Rebased onto current main again. The CI runs from the August push failed at startup on a workflow file issue and never ran any jobs, and the new ones are waiting on approval. Could a maintainer approve the workflows? |
| // Reject an oversized value before any bytes are written. The little-endian path below | ||
| // copies all `length` bytes into the fixed TYPE_WIDTH slot with unchecked native writes, | ||
| // so validating after the copy would leave adjacent memory already corrupted. | ||
| if (length > TYPE_WIDTH) { |
There was a problem hiding this comment.
BitVectorHelper.setBit(validityBuffer, index) a few lines above still runs before this check. If an oversized value is rejected here, the slot is already marked valid. A caller that catches the exception then sees isNull(index) == false and reads stale or zero bytes instead of null.
Could you move this check above the setBit call, so a rejected call leaves the vector unchanged?
| // Reject an oversized value before any bytes are written. The little-endian path below | ||
| // copies all `length` bytes into the fixed TYPE_WIDTH slot with unchecked native writes, | ||
| // so validating after the copy would leave adjacent memory already corrupted. | ||
| if (length > TYPE_WIDTH) { |
There was a problem hiding this comment.
Same issue here as in DecimalVector.setBigEndian. setBit runs before this check, so a value longer than 32 bytes leaves the slot marked valid after the exception.
Please validate the length first.
| handleSafe(index); | ||
| BitVectorHelper.setBit(validityBuffer, index); | ||
|
|
||
| if (length > TYPE_WIDTH) { |
There was a problem hiding this comment.
handleSoft(index) and setBit both run before this check. A rejected oversize call can still grow the vector and mark the slot valid.
Please validate length at the top of the method so a failed call has no side effects.
| handleSafe(index); | ||
| BitVectorHelper.setBit(validityBuffer, index); | ||
|
|
||
| if (length > TYPE_WIDTH) { |
There was a problem hiding this comment.
Same here as in DecimalVector.setBigEndianSafe.
Please validate length before handleSafe and setBit.
| assertThrows( | ||
| IllegalArgumentException.class, () -> decimalVector.setBigEndian(0, new byte[24])); | ||
|
|
||
| assertEquals(neighbor, decimalVector.getObject(1).unscaledValue()); |
There was a problem hiding this comment.
This covers the original bug, since the neighboring slot stays intact. It doesn't check the target slot after the exception.
Once the check is moved, please also assert assertTrue(decimalVector.isNull(0)) after the failed call. That would catch the validity-bit ordering issue.
A matching test for setBigEndianSafe would help too. It should assert isNull and that the value capacity is unchanged after the rejected call.
| assertThrows( | ||
| IllegalArgumentException.class, () -> decimalVector.setBigEndian(0, new byte[40])); | ||
|
|
||
| assertEquals(neighbor, decimalVector.getObject(1).unscaledValue()); |
There was a problem hiding this comment.
Same as the TestDecimalVector comment. Please assert isNull(0) after the rejected call, and add a setBigEndianSafe case.
Move the length check ahead of handleSafe/setBit so a rejected oversize call leaves the vector unchanged: the slot stays null and the vector is not grown. Add setBigEndianSafe tests and assert isNull after rejection.
|
Good catch on the ordering. Pushed a fix: the length check now runs first in all four methods, before setBit (and before handleSafe in the safe variants), so a rejected oversize call leaves the slot null and doesn't grow the vector. Also updated the tests per your note: both setBigEndian tests now assert isNull(0) after the rejected call, and I added setBigEndianSafe cases for DecimalVector and Decimal256Vector asserting isNull and unchanged value capacity. Full TestDecimalVector + TestDecimal256Vector run is green. |
3 participants
What's Changed
setBigEndian(int, byte[])onDecimalVectorandDecimal256Vectorbyte-swaps the value into the fixed-width slot with uncheckedMemoryUtilwrites and only checks the length afterwards, so a byte array longer than the type width overruns the slot into adjacent off-heap memory before theIllegalArgumentExceptionfires.setBigEndianSafe(int, long, ArrowBuf, int)does the same write with no length check at all. This moves the length check ahead of the write and adds it to the safe variant, so oversized input is rejected before any memory is touched; valid lengths are unaffected.Closes #1246.